I think it would be worth adding a test here that ensures this command produces the expected number or queries w/ assertNumQueries. example:

That makes sense! When I used this method, I see that there are a number of calls: https://github.com/edx/edx-exams/actions/runs/8394389300/job/22991407617?pr=255. Is it better to just check relative number of queries (for example, that batch size affects the number of queries for example) or is there a better way to finetune? I couldn't figure out a good way to constrain to only a certain type of query (for example, just the bulk query). Was poking around here: https://docs.djangoproject.com/en/5.0/topics/testing/tools/

You can probably just make this it's own test rather than baking it into the others as an extra assertion. For that one I'd try a bunch of extra rows to ensure the query count has not increased in an unexpected way based on more data.

For getting the expected number you should be able to walk though the code and count things up for what you expect. If it's different, it would be good to know why or where the extra query is coming from. There's a few ways to debug django queries but an easy one is to use

from django.db import connection
print(connection.queries)

Is this a good default size?

as a default I think maybe we keep the number lower in the hundreds or 1k max. Not sure when we'd start running into memory issues with a really big bulk create query. You could play with this locally to get an estimate however.

as a default I think maybe we keep the number lower in the hundreds or 1k max. Not sure when we'd start running into memory issues with a really big bulk create query. You could play with this locally to get an estimate however.

You can probably just make this it's own test rather than baking it into the others as an extra assertion. For that one I'd try a bunch of extra rows to ensure the query count has not increased in an unexpected way based on more data.

For getting the expected number you should be able to walk though the code and count things up for what you expect. If it's different, it would be good to know why or where the extra query is coming from. There's a few ways to debug django queries but an easy one is to use

from django.db import connection
print(connection.queries)

it would be better to test if the correct user/role was created here rather than assert any role exists. Same for the below tests

What made you choose the unicodecsv package? I ask because Python's native csv module has a DictReader class that should provide the same functionality. Looking at the docs for unicodecsv, it looks like it improves upon some unicode issues in Python 2, but we're on Python 3 here, so I'd imagine we don't benefit from unicodecsv. Where possible, I think it would be better to leverage native packages. I think this should be an easy swap without any impact to the command or its tests. What do you think?

As a nit, at least for the csv.DictReader class, the class is iterable, so you don't have to cast the value to a list; you can just iterate the instance. Maybe unicodecsv.DictReader is different? Oh, I see. You're casting it to a list because you're iterating reader twice and don't want to exhaust the iterable, right?

Ah, good point! I've changed to use csv. The only reason I used unicode was because I had referenced code from elsewhere... so not really a good reason haha.

I primarily cast to a list because I check the length of the reader (in order to batch the creates for the course staff). Lemme know if this is a bad idea though!

I'm not sure about the answer to this question, and maybe this is not possible. But I noticed that you create the users_existing set querying the User model via the username field. Here, though, you're querying the User model via the username and the email field. I'm wondering whether a scenario is possible where a user's would be found in users_existing but not in this query.

If we're pulling this data from the LMS, I think this could happen, because email is modifiable, but username isn't. I think email is maintained on our User model via syncing with the JWT when they hit edx-exams in the process of an exam, right? Do you think this is something we need to address here? I'm just not sure this code guarantees a user will exist, which could cause an exception here.

Since the function is in a transaction, it should be okay, because it'll rollback, we can correct the CSV, and try again, but it would be better to handle explicitly.

Oooh, that's a really good point. Since the username is unique, could I just get by username instead? I've updated to be that way but let me know if that could cause any issues.

I think that's good! username should not change.